feat(dgw): Kerberos credential injection via explicit jet_cred_id#1768
feat(dgw): Kerberos credential injection via explicit jet_cred_id#1768irvingouj@Devolutions (irvingoujAtDevolution) wants to merge 3 commits into
Conversation
89c47b0 to
d6e3e3b
Compare
There was a problem hiding this comment.
Pull request overview
Implements DGW-378 by introducing an explicit cred_injection_id minted at preflight and propagated as jet_cred_id in JWTs, then used to route Kerberos credential-injection traffic deterministically (not heuristically) for /rdp and /jet/KdcProxy.
Changes:
- Add
cred_injection_idto preflight provision-credentials requests/responses and threadjet_cred_idthrough Association/KDC token claims. - Refactor credential store to key entries by
cred_injection_id, attach per-session Kerberos material, and requiredst_hstto capture the target hostname for SPN validation. - Update Gateway KDC proxy handling and RDP proxy Kerberos loopback dispatch to use the explicit ID, and update tooling/.NET helpers accordingly.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs | Adds a typed DTO for provision-credentials including cred_injection_id. |
| utils/dotnet/Devolutions.Gateway.Utils/src/KdcClaims.cs | Adds optional jet_cred_id claim for KDC tokens. |
| utils/dotnet/Devolutions.Gateway.Utils/src/AssociationClaims.cs | Adds optional jet_cred_id claim for association tokens. |
| tools/tokengen/src/server/server_impl.rs | Accepts/forwards jet_cred_id in token generation server endpoints. |
| tools/tokengen/src/main.rs | Adds --jet-cred-id CLI option for relevant subcommands. |
| tools/tokengen/src/lib.rs | Serializes jet_cred_id into Association/KDC token claims. |
| devolutions-gateway/src/token.rs | Adds jet_cred_id to claims + helper extractors (extract_jet_cred_id, extract_dst_hst). |
| devolutions-gateway/src/credential/mod.rs | Refactors credential store indexing to cred_injection_id, adds injection state + per-session Kerberos material, enforces dst_hst presence for injection sessions. |
| devolutions-gateway/src/credential/tests.rs | Adds unit tests for insert/lookup/eviction and synthetic realm behavior. |
| devolutions-gateway/src/api/preflight.rs | Adds optional cred_injection_id input and returns provisioned-credentials output kind with the final id. |
| devolutions-gateway/tests/preflight.rs | Updates preflight test expectations to the new output kind and returned id. |
| devolutions-gateway/src/api/kdc_proxy.rs | Routes injection sessions via jet_cred_id lookup; forwards otherwise; factors realm enforcement. |
| devolutions-gateway/src/extract.rs | Adds KdcToken extractor to authenticate path-bound /jet/KdcProxy/{token} tokens. |
| devolutions-gateway/src/rdp_proxy.rs | Adds in-process KDC dispatch via http://cred.invalid/{id} and threads credential store into CredSSP server generator. |
| devolutions-gateway/src/rd_clean_path.rs | Switches injection lookup to jet_cred_id first, fallback to JTI index; threads credential store into proxy path. |
| devolutions-gateway/src/generic_client.rs | Same injection lookup precedence and threads credential store into RdpProxy. |
| devolutions-gateway/src/openapi.rs | Updates OpenAPI schema representation for cred_injection_id and new output kind. |
| devolutions-gateway/openapi/gateway-api.yaml | Adds cred_injection_id to preflight models and new provisioned-credentials kind. |
| devolutions-gateway/openapi/dotnet-client/.../PreflightOutputKind.cs | Adds provisioned-credentials enum variant. |
| devolutions-gateway/openapi/dotnet-client/.../PreflightOutput.cs | Adds cred_injection_id field to generated model. |
| devolutions-gateway/openapi/dotnet-client/.../PreflightOperation.cs | Adds cred_injection_id to generated model. |
| devolutions-gateway/src/api/webapp.rs | Initializes new claims fields (jet_cred_id: None) when signing tokens. |
| docs/plans/2026-04-27-dgw-378-explicit-identity-design.md | Adds design doc for explicit identity routing. |
| review.md | Adds an internal review write-up (likely not intended for merge). |
| review-followup.md | Adds follow-up internal review notes (likely not intended for merge). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Pavlo Myroniuk (TheBestTvarynka)
left a comment
There was a problem hiding this comment.
Wow, amazing feature 🤩. Per-session Kerberos material is a great improvement.
I reviewed the PR. LGTM.
There was a problem hiding this comment.
Thanks for tackling this! This is clearly moving in the right direction, and the test coverage you added on CredentialStore is appreciated. That said, I'd like us to be more conservative on three architectural points before this lands:
-
The
credentialmodule should not own Kerberos.SessionKerberos,build_session_kdc_config,principal_for_realm,kerberos_salt,synthetic_realm,encrypt_with_kerberos, and thedst_hstextraction insideinsertall turn what used to be a protocol-neutral store into a Kerberos-aware one. They belong in a newrdp_proxy::kerberos_session(or similar) submodule, withInjectionStatereduced to{ mapping, target_hostname }and the target hostname passed toinsertas a typed parameter from preflight (which already has the validated token). -
The new
cred_injection_idclaim/field is avoidable. The PR's own backwards-compat fallback (get_by_tokenkeyed by JTI) demonstrates that the association-token JTI is already a sufficient identifier. I'd like us to take the minimal-surface variant: keep ajet_cred_idclaim only onKdcTokenClaims, defined as "the JTI of the associated session token", i.e. it's a reference to another token's ID, not a reuse of one. With that, we drop:jet_cred_idonAssociationTokenClaimscred_injection_idrequest field onProvisionCredentialsParamsPreflightOutputKind::ProvisionedCredentials(revert toAck)InsertError::Conflictand the whole "caller proposes an ID" threat modelassociation_token_index+get_by_token- All the OpenAPI / .NET client additions tied to those
-
The
enable_unstablegate was silently removed. Whether proxy-based Kerberos injection is ready to be promoted out of the unstable gate is a separate decision that deserves its own PR and CHANGELOG entry. Please keep the gate aroundbuild_credential_injection_server_kerberos_confighere. Most likely we’ll simply open a PR once this lands.
A few smaller items in line comments.
| /// `tracing::debug!(?kerberos, ...)`. Access requires an explicit `expose_secret()` call, | ||
| /// which is greppable and reviewable. The custom [`fmt::Debug`] adds a second layer that | ||
| /// stays correct even if a future refactor changes the field types. | ||
| pub struct SessionKerberos { |
There was a problem hiding this comment.
Move all of this out of credential/. SessionKerberos, build_session_kdc_config (line 179), principal_for_realm, kerberos_salt, synthetic_realm, encrypt_with_kerberos, random_32_bytes, plus the hardcoded service_user_name: "jet" and max_time_skew: 300 — all of this is Kerberos/CredSSP session glue that has no business inside what used to be a protocol-neutral credential store.
Suggested layout: a new rdp_proxy::kerberos_session (or credssp_session) submodule owns the SessionKerberos type and the KDC config builder. InjectionState in credential/ becomes just { mapping, target_hostname }. The Arc<SessionKerberos> is either lazily derived on first CredSSP use, or stored in a parallel HashMap<Uuid, Arc<SessionKerberos>> owned by the RDP layer (keyed by the same UUID — see point 2 in the summary, that UUID should be the association-token JTI).
This also lets CleartextAppCredentialMapping::encrypt_with_kerberos go back to a plain encrypt, so non-Kerberos use of the store stops minting AES-256 keys it never uses.
There was a problem hiding this comment.
removd from credential/
refactored everything you mentioned here into credential_injection_kdc.rs
| kerberos.service_user_password.expose_secret(), | ||
| )); | ||
|
|
||
| let kdc_url = Url::parse(&format!("http://cred.invalid/{}", credential_entry.cred_injection_id)) |
There was a problem hiding this comment.
About the http://cred.invalid/<id> trampoline: I see why you reached for it (sspi-rs's CredSSP server config exposes kdc_url: Option<Url>, and you needed a way to dispatch a synthetic in-process "KDC"), but the implementation is heavier than the idea. Three concrete asks:
-
Verify whether sspi-rs offers a custom KDC dispatcher API (trait object, async closure,
KdcClient-like abstraction). If yes — use it; the URL trick goes away andCredentialStoreHandleno longer needs to be threaded throughperform_credssp_with_client→process_authentication_with_client→resolve_server_generator→send_network_request. -
If no clean API exists in sspi-rs, that's fine for now (sspi-rs is ours — Devolutions/sspi-rs), but please open a GitHub issue against sspi-rs asking for a pluggable KDC dispatcher and link it from a
// TODO(sspi-rs#NNN)comment here. We can keep a workaround in this PR to unblock; the cleanup lands when sspi-rs ships the API. -
Even with the workaround, please localize the interception. Right now
send_network_requestis a generic Kerberos-network helper that knows about"cred.invalid". Push the loopback dispatch into a non-generic function (closed overArc<SessionKerberos>+mapping+target_hostnamealready in scope atbuild_credential_injection_server_kerberos_config), and keep the generic helper unaware of the magic hostname.
Also related: kdc_proxy_http_client() (line 653) and the "http" | "https" non-loopback branch (line 681) appear to have no real caller in this PR — sspi-rs only emits tcp:///udp:// in the non-injection path and http://cred.invalid/... in the injection path. Either delete this branch or document the external use case it's meant to serve (an undocumented "session token can point at arbitrary HTTP host" capability would be a security concern).
There was a problem hiding this comment.
There was a problem hiding this comment.
Benoît Cortier (@CBenoit), currently, the sspi-rs does not have such an API. We need to implement it. I created an issue and described the situation there: Devolutions/sspi-rs#664. Feel free to edit and add more context.
irvingouj@Devolutions (@irvingoujAtDevolution), you can replace TODO(sspi-rs#NNN) with TODO(sspi-rs#664 in the code
There was a problem hiding this comment.
updated
| store.get_by_token(&token).is_none(), | ||
| "expired entry must not be returned by token" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Test coverage on CredentialStore mechanics is good. Three branches in this PR are still uncovered and I think are reachable from in-process tests (no real RDP client needed):
kdc_proxy::enforce_realm_token_matchbypass underdisable_token_validation.kdc_proxy::process_credential_injectionrealm-fallback when the envelope realm is missing.rdp_proxy::send_in_process_kdc_requestparses the URL path into a UUID, looks up the entry, and dispatches.
Full end-to-end CredSSP testing would need an actual RDP client and isn't the goal here — that's a welcome future improvement. Please add the easy-to-write ones if time permits; otherwise a tracked follow-up PR is fine.
There was a problem hiding this comment.
At a glance, it seems to me that this module should be left untouched, in favor of lazily deriving the necessary elements in the kdc_proxy module, when necessary. It’s possible to obtain the original token from the jet_cred_id claim in the KDC token (it’s in the CredentialEntry that we can retrieve from the credential store handle using this ID). It’s then possible to extract the target_hostname. This avoids all the eager logic of pre-extracting more stuff than actually necessary. Again, this module should stay protocol-agnostic for the most part, and the new InjectionState seems to be designed with RDP in mind. Since it does not seem necessary to store such extra information (we can retrieve that from the token lazily at the most relevant place), we should do so in order to keep the code clean. A side effect of that is that the preflight module does not need to change either. A side benefit is that it completely removes some invalid states, making the code overall more straightforward.
|
Benoît Cortier (@CBenoit), I'm doing an refactoring, I'll pin you once it's all ready and tested |
08244d3 to
4b91ac4
Compare
`provision-credentials` now validates the association token shape (JTI, dst_hst present, dst_alt empty) at preflight time and reserves a credential-injection context slot keyed on the token JTI. The Kerberos session and target hostname are derived lazily from the stored token on first CredSSP/KDC use, keeping `CredentialStore` strictly protocol-neutral while still failing fast on bad tokens. Renames `kerberos_session_store` to `credential_injection_context_store` to reflect that it now holds a per-session context (slot + lazy Kerberos session), not the session itself.
|
Benoît Cortier (@CBenoit) I refactored it a bit, now we have a proper in process kdc that collecting all the pieces together and unifies the logic of in process kdc and in process fake server. credential/mod.rs is now clean |
There was a problem hiding this comment.
This can be removed
Summary
Implements DGW-378: proxy-based Kerberos credential injection for Web Access. A new
cred_injection_id(UUID) is minted at/jet/preflight provision-credentialsand propagated through DVLS-issued association and KDC tokens as the JWT claimjet_cred_id— the gateway uses it as the primary credential-store key for/rdpand/jet/KdcProxy. Replaces the heuristic AS-REQ router from the experimentaldgw-378-session-redesignbranch.Coupled with DVLS-13821; old DVLS still works for NTLM injection via the JTI fallback (no regression).
Highlights
SessionKerberos(krbtgt + service long-term keys,secrecy-wrapped) with aCRED-{uuid}.INVALIDsynthetic realm for bare-UUID proxy usernames.http://cred.invalid/{cred_injection_id}(RFC 6761 reserved TLD) — no HTTP, no token, no middleware.dst_hst(TERMSRV/<target>) so client AP-REQ tickets validate against the impersonated server identity. Required at insert time — missing/malformeddst_hstis rejected at preflight, not mid-CredSSP.Verification
credential/tests.rs(insert / lookup / eviction / synthetic realm).Test plan